🎨 Palette: Add aria-pressed to toggle buttons - #165
Conversation
Co-authored-by: alvin000009238 <107313913+alvin000009238@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces aria-pressed attributes to the theme and password visibility toggle buttons to improve accessibility, along with corresponding test updates and static asset hash changes. However, the review identifies an accessibility anti-pattern where aria-pressed is used in conjunction with dynamic aria-labels that describe actions rather than states. This conflict can lead to confusing screen reader announcements, and it is recommended to either use a static label with aria-pressed or omit the attribute if the label already describes the state change.
| eyeOffIcon.style.display = 'block'; | ||
| togglePasswordBtn.setAttribute('aria-label', '隱藏密碼'); | ||
| togglePasswordBtn.setAttribute('title', '隱藏密碼'); | ||
| togglePasswordBtn.setAttribute('aria-pressed', 'true'); |
There was a problem hiding this comment.
Adding aria-pressed to a button that already changes its aria-label to describe an action (e.g., "隱藏密碼") is an accessibility anti-pattern. A toggle button should either have a static label with aria-pressed to indicate state, or a dynamic label that changes to describe the action without aria-pressed. Mixing both can lead to confusing announcements like "Hide password, toggle button, pressed". Consider keeping the label static if you want to use the toggle button pattern.
| const nextThemeLabel = isLight ? '深色' : '淺色'; | ||
| toggleBtn.setAttribute('aria-label', `切換至${nextThemeLabel}模式`); | ||
| toggleBtn.setAttribute('title', `切換至${nextThemeLabel}模式`); | ||
| toggleBtn.setAttribute('aria-pressed', isLight ? 'true' : 'false'); |
There was a problem hiding this comment.
Using aria-pressed alongside a dynamic aria-label that describes the next action (e.g., "切換至深色模式") creates a conflict between the button's label and its state. For instance, in light mode, the button would be announced as "Switch to dark mode, toggle button, pressed", which is contradictory. It is recommended to use a static label (e.g., "深色模式") when using aria-pressed, or simply omit aria-pressed if the label already describes the state change.
There was a problem hiding this comment.
Pull request overview
Adds aria-pressed to two custom toggle buttons (theme toggle + password visibility) so assistive tech can understand their on/off state, and updates the theme unit test accordingly.
Changes:
- Add
aria-pressedupdates infrontend/theme.jswhen applying light/dark theme. - Add
aria-pressedupdates infrontend/sync.jswhen toggling password visibility. - Add initial
aria-pressed="false"to the relevant buttons inpublic/index.htmland extend theme tests to assert the attribute.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/frontend/theme.test.js | Extends applyTheme assertions to cover aria-pressed. |
| public/index.html | Adds initial aria-pressed attributes to toggle buttons (also updates Vite asset hashes). |
| frontend/theme.js | Sets aria-pressed based on the active theme during applyTheme(). |
| frontend/sync.js | Sets aria-pressed based on whether the password is currently visible. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const nextThemeLabel = isLight ? '深色' : '淺色'; | ||
| toggleBtn.setAttribute('aria-label', `切換至${nextThemeLabel}模式`); | ||
| toggleBtn.setAttribute('title', `切換至${nextThemeLabel}模式`); |
| togglePasswordBtn.setAttribute('aria-label', '隱藏密碼'); | ||
| togglePasswordBtn.setAttribute('title', '隱藏密碼'); | ||
| togglePasswordBtn.setAttribute('aria-pressed', 'true'); | ||
| } else { | ||
| eyeIcon.style.display = 'block'; | ||
| eyeOffIcon.style.display = 'none'; | ||
| togglePasswordBtn.setAttribute('aria-label', '顯示密碼'); | ||
| togglePasswordBtn.setAttribute('title', '顯示密碼'); |
| togglePasswordBtn.setAttribute('aria-label', '隱藏密碼'); | ||
| togglePasswordBtn.setAttribute('title', '隱藏密碼'); | ||
| togglePasswordBtn.setAttribute('aria-pressed', 'true'); | ||
| } else { | ||
| eyeIcon.style.display = 'block'; | ||
| eyeOffIcon.style.display = 'none'; | ||
| togglePasswordBtn.setAttribute('aria-label', '顯示密碼'); | ||
| togglePasswordBtn.setAttribute('title', '顯示密碼'); |
| assert.equal(moon.classList.contains('hidden'), true); | ||
| assert.equal(btn.getAttribute('aria-label'), '切換至深色模式'); | ||
| assert.equal(btn.getAttribute('title'), '切換至深色模式'); | ||
| assert.equal(btn.getAttribute('aria-pressed'), 'true'); |
💡 What: Added the
aria-pressedattribute to the theme toggle and password visibility toggle buttons.🎯 Why: To explicitly communicate the active/inactive state of these custom toggle buttons to screen readers, improving accessibility for users who rely on assistive technologies.
📸 Before/After: N/A (non-visual change)
♿ Accessibility: Improves screen reader experience by explicitly communicating the state of custom toggle buttons using the
aria-pressedattribute.PR created automatically by Jules for task 4857976360675655665 started by @alvin000009238